Skip to content

Fix crash when assigning to bool class fields#95

Merged
dcodeIO merged 1 commit intoAssemblyScript:masterfrom
alangpierce:fix-bool-assignment-crash
May 5, 2018
Merged

Fix crash when assigning to bool class fields#95
dcodeIO merged 1 commit intoAssemblyScript:masterfrom
alangpierce:fix-bool-assignment-crash

Conversation

@alangpierce
Copy link
Contributor

Fixes #94

With Type.bool, size is 1, so dividing by 8 isn't the right way to convert to
byteSize. Instead, we need to use the byteSize property that does the proper
ceiling division.

I left out the allocation from the test so that the resulting wat files are
easier to read.

@alangpierce
Copy link
Contributor Author

@dcodeIO I see that the dist files are in source control and a number of your recent changes update them, but the contributing guidelines say to exclude them. Should I be updating them in PRs or leaving them out? Maybe they should just be removed from source control and added to gitignore?

@MaxGraey
Copy link
Member

MaxGraey commented May 5, 2018

dist files needs for external tools like WebAssembly Studio which download relevant version. But you shoudn't update it in PR.

@alangpierce
Copy link
Contributor Author

Got it, so the contributing docs (https://github.com/AssemblyScript/assemblyscript/blob/master/CONTRIBUTING.md#submitting-pull-requests) are out of date? I guess when this project gets published to npm, WebAssembly Studio can just use that instead of relying on dist files in the repo?

@MaxGraey
Copy link
Member

MaxGraey commented May 5, 2018

This better ask to @dcodeIO.

@alangpierce alangpierce force-pushed the fix-bool-assignment-crash branch from 03e106c to 6fd2539 Compare May 5, 2018 18:38
@alangpierce
Copy link
Contributor Author

Ok, I updated the commit to include dist file updates as well (just the result of npm run build).

@alangpierce
Copy link
Contributor Author

Oh, funny, the build failed with "The pull request includes changes to distribution files, but it shouldn't."

I'll leave them out I suppose.

@alangpierce alangpierce force-pushed the fix-bool-assignment-crash branch from 6fd2539 to 9046e97 Compare May 5, 2018 18:41
@MaxGraey
Copy link
Member

MaxGraey commented May 5, 2018

No, you should exclude dist files) So everything was alright before

But you shoudn't update it in PR.

Fixes AssemblyScript#94

With `Type.bool`, size is 1, so dividing by 8 isn't the right way to convert to
byteSize. Instead, we need to use the `byteSize` property that does the proper
ceiling division.

I left out the allocation from the test so that the resulting wat files are
easier to read.
@alangpierce alangpierce force-pushed the fix-bool-assignment-crash branch from 9046e97 to e3ffa4e Compare May 5, 2018 20:53
@alangpierce
Copy link
Contributor Author

@dcodeIO updated to include a load in the test case as well.

@dcodeIO dcodeIO merged commit ce2bf00 into AssemblyScript:master May 5, 2018
@dcodeIO
Copy link
Member

dcodeIO commented May 5, 2018

Thank you! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants